Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ScalaIDE container to the Eclipse classpath for Scala projects #99

Closed
wants to merge 1 commit into from

Conversation

ben-manes
Copy link
Contributor

The ScalaIDE plugin is the standard approach for developing Scala projects in Eclipse. Previously an import of a Scala gradle project would initially fail to build due to the error 'Cannot find Scala library on the classpath. Verify your build path!'. This had to be resolved manually so that the ScalaIDE verifier could be run successfully.

The ScalaIDE is now added as a classpath container. In addition to the unit test, the change was verified on a private project. The container was added to Scala projects only (and not Java projects).

Further background details is provided in the forum post, Generate ScalaIDE Eclipse metadata for Scala projects.

@mockitoguy
Copy link
Contributor

Hey!

Thanks for the pull request. Can you double check how this implementation behaves in this scenario:

  1. the gradle scala project already declares dependency to scala-library with a different version than the scala-library shipped with Scala IDE.
  2. same as above but let's say gradle project depends on matching version of scala-library.

AFAIK, the scala container only adds the 'scala-library' on the path. If it also adds other dependencies, we need to make sure the project imports smoothly if the Gradle project already declares similar dependencies explicitly.

@ben-manes
Copy link
Contributor Author

Can you please clarify the expectations between disjoint versions?

Scala only provides backwards compatibility for minor versions (e.g. 2.9.0 to 2.9.2). It does not guarantee compatibility between major revisions (e.g. 2.9.x to 2.10.x). While much of the core is expected to stabilize as the language ages, most cross-version mixing is unsafe. Using a version of the ScalaIDE that does not match the major version in the build file would be considered an invalid configuration. This is why the ScalaIDE includes a classpath validator (see faq), which seems to be all that the container adds. The compatibility problem is explained by Suereth and Odersky, with coding strategies to avoid issues.

I'll try your scenarios tomorrow.
Thanks!

@mockitoguy
Copy link
Contributor

I want to make sure that the project imports cleanly to eclipse if it already has scala library. I don't have any specific versions in mind. Cheers!

@ben-manes
Copy link
Contributor Author

  1. gradle dependency v2.9.2 and ScalaIDE adding v2.9.1 to the referenced libraries.
    • Import works correctly. I see the mixed versions in the project, but no failures occur
    • Navigating into scala library code resolves to the ScalaIDE's library, which could cause developer confusion
  2. same versions
    • This was my project's default, so verified.

A little searching and an additional concern was raised on the ScalaIDE forum. I have not encountered the specs2 issue, but was able to verify that full rebuilds are being performed instead of incremental. What do you think the proper fix is for that issue?

@hansd
Copy link
Member

hansd commented Sep 6, 2012

Right now Gradle does not do incremental compiles for Scala. We are aware that this is an important issue. We have already successfully spiked a solution by integrating with the SBT compiler. Right now we are planning to tackle this feature in Gradle 1.3 with a high priority. So possibly there will be a solution in a couple of weeks.

@hansd hansd closed this Sep 6, 2012
@hansd hansd reopened this Sep 6, 2012
@ben-manes
Copy link
Contributor Author

The incremental compilation referred to is through the Eclipse Scala Builder, which is backed by an embedded copy of SBT. The generated Eclipse metadata causes the ScalaIDE to not perform incremental compilation. I think that it is separate from gradle native support. It may be a ScalaIDE bug depending on interpretation.

@mockitoguy
Copy link
Contributor

Hey Ben,

I'm not convinced that having 2 conflicting versions of scala-library on the IDE build path is healthy. You've tested it and apparently it does not break anything. I had problems with that few months ago (I don't remember what version of scala IDE / scala-library I have used) - the build in IDE didn't work in that scenario (however, I don't remember what was the exact problem). Anyway, to fix the problem I needed to remove the container from the project.

Not sure what to do with the problem that the scala ide requires its very own version of scala library for the incremental compilation to work. Is this a bug that the scala IDE team wants to fix at some point?

I'm wondering about:

  1. Gradle only adds the container if scala-library is not already on build classpath for given project.
  2. Gradle removes from the build path the reference to scala-library (defined via regular gradle dependencies) and adds the container instead.

I need touch base with the team how we want to approach it.

@ben-manes
Copy link
Contributor Author

Hi Szczepan,

I suspect that you used disjoint versions (e.g. 2.9.x in Gradle, 2.10.x in Eclipse). That would be a really easy mistake to make by naively taking the latest version and not realizing the incompatibility issues. I haven't tested that scenario since it could lead to a lot of surprising failures.

I definitely agree with (2) and I can try to make those changes to my pull request if agreed to. I'm less convinced of (1) because I haven't seen the problem occur.

When your team decides on the preferred approach, we can ask on the Scala IDE mailing list for one of their developers to offer feedback.

@mockitoguy
Copy link
Contributor

Hey,

We're leaning towards solution 2 but we haven't yet concluded :)

When your team decides on the preferred approach, we can ask on the Scala IDE mailing list for one of their developers to offer feedback.

Are you already on this mailing list? Can lead the effort and ask the Scala IDE guys?

Cheers!

@ben-manes
Copy link
Contributor Author

requested feedback from ScalaIDE team.

@dragos
Copy link
Member

dragos commented Sep 12, 2012

I'm a Scala IDE committer. I'm very glad to see support from Gradle!

  • The incremental builder should not misbehave if you have a second (or a different) scala-library on the build path. If it does, it's a bug and we'll fix it. If you have a small project I can have a look.
  • If there's more than one library on the classpath, the first one is picked up (the order can be changed on the Order and Export tab of the build path window
  • Hyperlinking does not treat the scala library in any special way, so the order on the classpath applies here as well.

I don't know if Gradle already does it, but it would be great if it attached sources to jars, so in case the exported project definition does not have the Scala container, the scala-library.jar comes with sources and hyperlinking still works.

For reference, sbteclipse filters out the scala-library dependency and adds only the classpath container. People seem to be happy with this solution, and it has the advantage that if you have the wrong IDE version (2.9 instead of 2.10), it won't crash spectacularly: you would see compilation errors if you're using anything that's been added in 2.10), or for dependencies that are compiled against the wrong version.

@ben-manes
Copy link
Contributor Author

The ScalaIDE 2.9x nightly appears to work perfectly. This is with Scala 2.9.2 gradle dependency and Scala 2.9.3-(nightly #) as the Scala Library.

I attempted to revert back to a stable release and reproduce the problem, but forgetting my original version I was unable to confidently identify a problem in v2.0.2. I was assuming full vs. incremental compiles for a project based on the speed of the build. If the ScalaIDE and gradle dependency used the same version of Scala then the build was immediate. If they differed by a minor version then the build was much slower. The change was to modify a single character in a Scalatra route. Given the behavior of the nightly build, I don't consider this to be a problem anymore.

I am in favoring of being consistent the rest of the ScalaIDE ecosystem. The m2eclipse-scala plugin also removes the scala-library from its dependency list.

With respect to attaching sources to jars, the EclipseClasspath has downloadSources enabled by default ("Whether to download and add sources associated with the dependency jars. Defaults to true.").

@mockitoguy
Copy link
Contributor

Thanks a lot for the research and feedback.

Let's go for the option with replacing the scala-library with the scala container.

@ben, can you prepare a new pull request? :) Please remember about:

  1. Eclipse plugin already does something for the scala projects - can you keep your code close to it?
  2. Unit test should cover that we're adding the container for scala projects and not adding for some other project, let's say java. It should also cover that the order of applying eclipse/scala plugin does not matter (in your test, just apply eclipse before scala and that will do it).
  3. We should have an integration test (say a new method in EclipseClasspathIntegrationTest) with a build file that has eclipse/scala plugins applied at the top of the script and dependencies declared later on, including scala library. Use fake maven repo (you'll find examples in the existing tests). Implementation-wise you probably want to use 'whenMerged' hook to remove scala library from the classpath.
  4. Can you document in the user guide (eclipse plugin I think) what we do for scala projects?
  5. Can you add a short note to the release notes ('notes.md') file about the feature?

Thanks a lot!

@ben-manes
Copy link
Contributor Author

Thanks Iulian and Szczepan!

I'd like to keep this pull request open until I have the code changes ready. I'll then close this request and start the new one so that the discussions can focus on the code review. That way these improvements don't fall off the radar as we work on the coding.

I will be participating in college career fair events next week so my changes may be delayed. Please note that since I am unaffiliated with Gradle or Scala teams, feel free to push forward if my progress becomes too slow. I'll work on the five points opportunistically, but may have other commitments that take precedence.

@mockitoguy
Copy link
Contributor

Hey @ben,

There's no rush. Feel free to work on the items in whatever order / time you like. We won't push on the implementation because of other priorities on our end. Bear in mind that we'd rather pull the request if all the items are completed (e.g. integ tests, docs, etc :)

It would be really helpful if you used fresh fork so that we don't have to review old commits.

Thanks a lot and good luck!

@ben-manes
Copy link
Contributor Author

I finally got back to this and have a partial solution. The only way I could find to remove the dependencies within EclipsePlugin.gradle was to use an whenMerged block (see changes). This removes it from the generated .classpath as desired.

However, when importing with the Gradle STS plugin the dependencies are still listed. I suspect that it is using the Gradle Tooling API instead to retrieve the list of external dependencies.

I'm unsure how best to proceed so that the removal of the external dependencies can be seen by gradle tooling.

@ben-manes
Copy link
Contributor Author

I realized a better approach that works is effectively,
eclipse.classpath.minusConfigurations += configurations.scalaTools

The only problem with this is that the scalaTools includes different dependencies than the ones that the ScalaIDE adds, so I have to dynamically create a configuration object. I'm trying to figure out the best way now.

It would be very helpful if a Gradle dev was available to chat for guidance. The freenode irc channel is unresponsive.

@ben-manes
Copy link
Contributor Author

@szczepiq

I have finished all items, rebased my branch, and squashed all changes into a single commit. Please let me know if you'd still prefer that we close this pull request and open a new one to focus on the code. I'm happy to continue this one.

I decided to add a scalaIde configuration to allow projects to exclude the scala library dependencies provided by the ScalaIDE. This was after a few failed attempts:

  • It appears that whenMerged changes are not made visible to the Gradle STS plugin. This approach had the benefit of automatic filtering, similar to the Maven plugin's approach linked above. I suspect that the changes are too late and opaque that they are not communicated out via the Gradle Tooling API.
  • The Scala plugin's scalaTools configuration could not be used instead of the scalaIde. If used directly it would exclude additional dependencies not provided by the ScalaIDE.
  • Due to non-deterministic ordering, I was unable to find a reliable way to inspect dependencies to infer the scala version. This meant that I could not dynamically create a detached configuration to exclude the libraries via a minusConfiguration.
  • Because of the above, I also attempted to specify the dependencies as a version range (e.g. 2.+). I had hoped that a minusConfiguration might have this be treated like a filter, but this did not appear to work.

The only two solutions remaining was to either have the build file specify the Scala version or add a new configuration type. I am unsure why the Scala plugin preferred the latter approach, but it set the precedent and expectation of how the Scala project should be configured. This is similar to how the idea-scala-facet plugin added a scalaApi configuration in order to perform some of the same integration magic.

A benefit of the approaches taken is that I was able to add them as build customizations into my projects until Gradle includes the improved support. The build file now emulates these fixes by including,

eclipse.classpath {
  minusConfigurations += configurations.scalaIde
  containers += ["org.scala-ide.sdt.launching.SCALA_CONTAINER"]
}

@mockitoguy
Copy link
Contributor

Thanks for the investigation. I need to think about it a bit more.

@ben-manes
Copy link
Contributor Author

Please let me know if there is anything else that I can do so that v1.3 contains ScalaIDE support. That could be either the proposed code changes, documenting only the build customizations, or another alternative. I'd like to see a documented fix in v1.3 due to the awesome work towards native incremental compilation support. The build customizations has been a positive improvement for my engineering team by making Eclipse much more usable.

@mockitoguy
Copy link
Contributor

Hey Ben,

Due to non-deterministic ordering, I was unable to find a reliable way to inspect dependencies to infer the scala version

I've discussed this issue with Peter. We think it should be possible to configure the minusConfigurations with a detached configuration that contains the same scala library dependency as the declared by the user. What exactly was the problem?

In general we would like to follow above path. E.g. detached configuration with scala library, assigned to minusConfigurations.

@ben-manes
Copy link
Contributor Author

Updated to use a detached configuration by waiting until all projects were evaluated before inspecting the dependencies. In addition to the tests, I verified with a scala project by inspecting the .classpath generated by ./gradlew eclipse as well as importing the project with Gradle STS. Both show the desired behavior.

This is thanks for Peter's answer to a similar problem on StackOverflow, where he advised using project.gradle.projectsEvaluated { ... } to resolve the ordering issue that I encountered previously.

The ScalaIDE classpath container is now added when to projects using
the Scala plugin. This resolve build problems after importing a Scala
project due to the ScalaIDE not being able to add its dynamically
computed entries to the classpath of the project.

The ScalaIDE adds the Scala libraries to the classpath, which
duplicates the dependencies provided by Gradle. In the ScalaIDE v2.0.2,
having multiple entries for scala-library results in full builds
instead of incremental). A detatched configuration allows projects to
exclude the dependencies from being provided by Gradle, which matches
the SBT and Maven behavior.
@pniederw
Copy link
Contributor

pniederw commented Nov 5, 2012

Thanks for the pull request. It's part of Gradle 1.3.

@pniederw pniederw closed this Nov 5, 2012
ov7a pushed a commit that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants